Skip to content

fix(graph): refresh preserved dependent nodes on increment (PR-P4)#343

Merged
HumanBean17 merged 1 commit into
masterfrom
fix/incremental-node-property-refresh
Jun 22, 2026
Merged

fix(graph): refresh preserved dependent nodes on increment (PR-P4)#343
HumanBean17 merged 1 commit into
masterfrom
fix/incremental-node-property-refresh

Conversation

@HumanBean17

Copy link
Copy Markdown
Owner

Summary

Follow-up to the init/increment-perf plan (PR-P1/P2/P3, #340#342). A review fan-out over the landed code found that PR-P2 introduced a latent data-correctness regression: converting _write_nodes_impl to bulk COPY FROM replaced the per-row _MERGE_SYMBOL upsert with a skip-if-exists filter, which silently dropped the property refresh on preserved dependent type nodes.

A preserved dependent's role/capabilities depend on project-wide inputs (meta-annotation chain, brownfield overrides) and can shift without the dependent's own source changing. The _delete_file_scope contract (issue #305) deliberately preserves dependent nodes and relied on that re-MERGE to refresh them in place. With the skip filter, the role was recomputed and then discarded — so the incremental graph diverged from a full rebuild until the next full rebuild. Roles drive semantic-search ranking, so this silently degraded results. No crash, no node/edge loss, self-heals on full rebuild. CI green — the two tests meant to guard incremental≡full equivalence were broken.

This PR fixes the regression and the tests that let it ship.

How it was found

Fanned out four reviewer subagents over the landed plan. Two analyzed the same code path and disagreed (one said HIGH bug with a repro, one said benign/unreachable). Adjudicated empirically: the "benign" verdict came from a reviewer that couldn't run ladybug in its env (bare python — the system-shadows-venv trap) and over-reasoned; the "bug" verdict was confirmed with a deterministic repro:

initial full (V1)           : Svc.role = OTHER     ✓
increment (V2, in scope)    : Svc.role = OTHER     ← stale
full rebuild (V2, truth)    : Svc.role = SERVICE

Changes

build_ast_graph.py

  • TypeIndexEntry / MemberEntry get a loaded_from_db flag set by _load_existing_{types,members}, distinguishing DB-loaded stubs (placeholder decls used only for cross-file resolution) from freshly-parsed scoped/dependent decls.
  • _write_nodes_impl: after the bulk COPY of new nodes, re-SET every mutable Symbol field on preserved, non-stub, type-kind nodes (_SET_SYMBOL_BY_ID), restoring the upsert. Stubs are skipped (their stored values are authoritative); non-type kinds carry no mutable role/capabilities; the full path is unaffected (empty DB → no existing nodes → no SETs).
  • _populate_declares_rows skips loaded members: their DECLARES edges already persist and re-emitting duplicated them (REL tables carry no PK). This second incremental≠full divergence surfaced once the equivalence test was fixed (incremental=7 vs full=5 DECLARES).
  • Delete dead _existing_symbol_ids (zero callers); correct the misleading Route-MERGE loop comment (it iterates all routes, not phantoms).

Tests

  • test_incremental_bulk_write_equivalent_to_full_rebuild now asserts real incremental≡full equivalence (node count, per-type edge counts, type roles) instead of the prior tautology (set(edge_type_keys) == set(edge_type_keys) + count > 0, which never failed because every rel type yields a count row even at 0).
  • New test_incremental_refreshes_dependent_role_on_meta_chain_change guards the exact regression.
  • Rename test_bulk_write_edges_match_per_row_baselinetest_bank_chat_bulk_build_matches_committed_baseline: the per-row path is gone, so the bulk-generated fixture is a drift anchor, not a per-row equivalence proof.

Validation

  • .venv/bin/ruff check . — clean
  • .venv/bin/python -m pytest tests -q848 passed, 14 skipped (HEAVY-gated)
  • bank-chat fixture rebuild + meta: ontology_version 17 (unchanged), parse_errors 0, all counters present
  • Repro (/tmp/repro_role_staleness2.py): increment now yields SERVICE = full rebuild (no divergence)

No ontology bump, no schema change, no re-index required, no public-surface change.

Known follow-ups (out of scope)

  • Converting the remaining per-row Route MERGE to bulk (needs existing-id filtering to reproduce the dedup).
  • OVERRIDES may have an analogous cross-file duplication; no test catches it yet.
  • Editing an annotation definition does not pull annotation-users into incremental scope (a pre-existing scope-tracking gap, distinct from the upsert regression fixed here).

🤖 Generated with Claude Code

PR-P2 replaced the per-row `_MERGE_SYMBOL` upsert in `_write_nodes_impl` with a
skip-if-exists filter, which dropped the property refresh on preserved dependent
type nodes. A dependent's `role`/`capabilities` depend on project-wide inputs
(meta-annotation chain, brownfield overrides) and can shift without its own
source changing, so the incremental graph diverged from a full rebuild — a
preserved dependent whose role lever changed stayed stale until the next full
rebuild. The `_delete_file_scope` contract (issue #305) relied on that re-MERGE
to refresh preserved dependents in place.

Found by fanning out reviewer subagents over the landed init/increment-perf plan
and adjudicating a B-vs-D conflict empirically (the "benign" verdict came from a
reviewer that couldn't run ladybug; the "bug" verdict reproduced).

Changes:
- Mark `TypeIndexEntry`/`MemberEntry` `loaded_from_db` when
  `_load_existing_{types,members}` creates them, so DB-loaded stubs (placeholder
  decls) are distinguishable from freshly-parsed scoped/dependent decls.
- After the bulk COPY of new nodes, re-SET every mutable Symbol field on
  preserved, non-stub, type-kind nodes (`_SET_SYMBOL_BY_ID`), restoring the
  upsert the design relied on. Stubs are skipped (their stored values are
  authoritative); non-type kinds carry no mutable role/capabilities; the full
  path is unaffected (empty DB → no existing nodes).
- `_populate_declares_rows` skips loaded members: their DECLARES edges already
  persist and re-emitting duplicated them (REL tables carry no PK). This second
  incremental≠full divergence surfaced once the equivalence test was fixed.
- Delete dead `_existing_symbol_ids` (zero callers); correct the Route-MERGE
  loop comment (it iterates all routes, not phantoms).

Tests:
- `test_incremental_bulk_write_equivalent_to_full_rebuild` now asserts real
  incremental≡full equivalence (node count, per-type edge counts, type roles)
  instead of the prior tautology (`set(keys)==set(keys)` + `count>0`).
- New `test_incremental_refreshes_dependent_role_on_meta_chain_change` guards
  the exact regression.
- Rename `test_bulk_write_edges_match_per_row_baseline` →
  `test_bank_chat_bulk_build_matches_committed_baseline`: the per-row path is
  gone, so the fixture (bulk-generated) is a drift anchor, not a per-row proof.

Known follow-ups (out of scope): converting the remaining per-row Route MERGE
to bulk; OVERRIDES may have an analogous duplication for cross-file pairs;
annotation-definition edits don't pull annotation-users into incremental scope
(a separate, pre-existing scope gap, not the upsert regression fixed here).

Co-Authored-By: Claude <noreply@anthropic.com>
@HumanBean17 HumanBean17 merged commit 82ea014 into master Jun 22, 2026
1 check passed
@HumanBean17 HumanBean17 deleted the fix/incremental-node-property-refresh branch June 22, 2026 19:59
HumanBean17 added a commit that referenced this pull request Jun 22, 2026
)

All of the init/increment-perf work has landed — the original plan
(PR-P1..P3: #340 cached ignore, #341 _write_edges bulk, #342 nodes/routes
bulk) and the post-review follow-ups (PR-P4 #343 dependent refresh +
DECLARES dedup, PR-P5 #344 annotation-scope fix + route bulk + overrides
invariant), plus its proposal (#338). Relocate the plan, agent-prompts,
and proposal from active/ to completed/, matching the Ladybug/INDEX-OUTPUT
close-out convention (pure rename, no content edits).

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant